I know we haven't reached to a conclusion on this, but based on the implementations of Sentry and Bugsnag, I decided to drop the usage of domain and simply opt for a try/catch block. What do you think? @shalvah @joshuap

Well done @subzero10 . I've got some thoughts, but I'm currently on holiday, so it'll have to be next week

I agree with you that a try/catch is enough, but I wonder about context (which I think is what domains were used for). For instance, in this handler.js:

const hb = require('honeybadger');

module.exports.handler1 = () => {
  hb.context({a: "1"});
});

module.exports.handler1 = () => {
  hb.context({a: "2"});
});

can we be sure these context calls won't clash if both these functions get executed, considering the same Lambda instance might be reused? I think we should add a test for that. If so, we could (should) use AsyncLocalStorage and polyfills to fix it.

I agree with you that a try/catch is enough, but I wonder about context (which I think is what domains were used for). For instance, in this handler.js:

const hb = require('honeybadger');

module.exports.handler1 = () => {
  hb.context({a: "1"});
});

module.exports.handler1 = () => {
  hb.context({a: "2"});
});

can we be sure these context calls won't clash if both these functions get executed, considering the same Lambda instance might be reused? I think we should add a test for that. If so, we could (should) use AsyncLocalStorage and polyfills to fix it.

I was worried that there would be a performance penalty with AsyncLocalStorage, but it seems this is not the case anymore:

• https://github.com/PhakornKiong/ALS-CLS-Benchmark
• AsyncLocalStorage kills 97% of performance in an async environment nodejs/node#34493

Still, I am wondering if we could just use a dictionary and store honeybadger context using key context.awsRequestId. Wouldn't that be enough?

The dictionary approach should work, but wouldn't that possibly lead to a memory leak? We'll keep adding to the dictionary and never remove unless there's a crash. I don't know how long Lambda instances stick around for, though...🤔

The dictionary approach should work, but wouldn't that possibly lead to a memory leak? We'll keep adding to the dictionary and never remove unless there's a crash. I don't know how long Lambda instances stick around for, though...🤔

Good question. I wonder if there's a way to find that out. I will do some googling.

Also, question that came up now: Even if the lambda instance is re-used, would it be the same nodejs process? If not, we wouldn't need to do anything here right?

Yeah, if it's a different process,, we would be safe. But considering that the reused instance already has the state initialised, I'm leaning towards same process.

Hmm. https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html#runtimes-lifecycle-shutdown

I read through the code, looking good. I added a few questions. @subzero10 are you still having issues with the setTimeout errors? I think that may be related to why we were using domains.

Yes, I am getting this error when throwing an uncaught error in a setTimeout:

Unknown application error occurred

I'm still investigating.
Update: It seems that AWS Lambda has a default listener that exits the process before it reaches our uncaughtException listener. By removing the first uncaughtException listener (only when running in AWS Lambda), errors are caught in our listener and reported to Honeybadger.

For the rest, there were times where I used to get this error:

Error report failed: an unknown error occurred. message=Client network socket disconnected before secure TLS connection was established

I think it was because I was throwing the error (therefore ending the aws lambda process) before the http request to Honeybadger is completed.
I updated the code to return a promise (that rejects) inside the catch block: https://github.com/honeybadger-io/honeybadger-js/pull/680/files#diff-5d4654b27960ff4b6441a43aa555e9be5ba6dfca79303d6abd464a077e38bcf2R49

can we be sure these context calls won't clash if both these functions get executed, considering the same Lambda instance might be reused? I think we should add a test for that. If so, we could (should) use AsyncLocalStorage and polyfills to fix it.

@shalvah @joshuap

Should we implement this in this PR? I'm thinking that this potential issue already exists with the current lambdaHandler implementation, so I'm wondering if AsyncLocalStorage is out of the scope of this PR: callback based to promise based lambda handler.
I'm only suggesting a separate PR/ticket because I like PRs to be as small as possible.

can we be sure these context calls won't clash if both these functions get executed, considering the same Lambda instance might be reused? I think we should add a test for that. If so, we could (should) use AsyncLocalStorage and polyfills to fix it.

@shalvah @joshuap

Should we implement this in this PR? I'm thinking that this potential issue already exists with the current lambdaHandler implementation, so I'm wondering if AsyncLocalStorage is out of the scope of this PR: callback based to promise based lambda handler. I'm only suggesting a separate PR/ticket because I like PRs to be as small as possible.

I tried to start work on AsyncLocalStorage, but it seems that it won't be that simple to implement (or maybe I should say there will be enough changes to have on a separate PR). We will have to do some refactoring to externalize the store for context and breadcrumbs.

Will review over the weekend @subzero10 . Sorry for the delay.

@shalvah @joshuap I think this is ready. I created this issue to tackle context isolation. I already had a solution in mind about this, but now that we return both async and sync versions of the handler I will have to think about it again 😅

@joshuap @shalvah I added notifyAsync documentation, see PR here.

@joshuap I think this is safe to merge and release, unless you have any other feedback.